fix(automl): pass params to underlying client #9794
Conversation
googlebot
added
the
cla: yes
Co-Authored-By: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>
| Additional domain-specific parameters, any string must be up to | ||
| 25000 characters long. | ||
| ``feature_importance`` - (boolean) Whether | ||
| [feature\_importance][[google.cloud.automl.v1beta1.TablesModelColumnInfo.feature\_importance] |
There was a problem hiding this comment.
Non-blocking: curious if there is documentation / spec about how to how to write docstring with links.
E.g.,
- why
\infeature\_importanceis needed, - what does
(-s)mean, and - seems different from markdown, [text][[link] instead of [text][link] is used.
There was a problem hiding this comment.
This formatting is from the proto comments. I believe it's some flavor of markdown that makes the comments render nicely on cloud.google.com. See https://github.com/googleapis/googleapis/tree/master/google/cloud/automl/v1 and https://cloud.google.com/automl/docs/reference/rpc/google.cloud.automl.v1.
When you need to add docstrings, please follow the Google Python Style Guide
|
LGTM |
|
Regarding "Is this something to be watching out for every time there's an update to AutoML?". |
|
@helinwang Would it be possible to write some tests to help catch these incompatibilities? |
|
@busunkim96 I think it's hard to do because: Do you know if there is plan to mirrow this repo into piper? I think it will solve this problem. We can setup integration test that reference the piper mirror code with TAP. |
|
@busunkim96 please merge if it looks good to you |
|
You're free to setup a mirror if you'd like, but that is not something the client libraries team is currently staffed to do. I'll make sure to tag both of you in future PRs so you can manually inspect changes. |
|
@sirtorry Could you do |
|
Thanks, @busunkim96 . |
|
I missed this earlier, but it looks like the unit tests are failing. |
There was a problem hiding this comment.
LGTM. Torry could you update the unit test?
The test is failing with errors like:
value = AssertionError("expected call not found.\nExpected: predict('my_model', {'row'..., {'row': {'values': [{'string_value': '1'}, {'string_value': '2'}]}}, None)",)
from_value = None
def raise_from(value, from_value):
> raise value
E AssertionError: expected call not found.
E Expected: predict('my_model', {'row': {'values': [{'string_value': '1'}, {'string_value': '2'}]}})
E Actual: predict('my_model', {'row': {'values': [{'string_value': '1'}, {'string_value': '2'}]}}, None)
|
could also add unit tests for non-empty param passing if y'all want |
|
I think that's fine, since the method just forward param to predict. |
passes params from tables_client to gapic prediction_service_client